Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 09, 2022 at 10:47:06AM +0000, Rahul Singh wrote:
> Hi Roger,
> 
> > On 9 Mar 2022, at 10:16 am, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > 
> > On Wed, Mar 09, 2022 at 10:08:12AM +0000, Rahul Singh wrote:
> >> Hi Jan,
> >> 
> >>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>> 
> >>> On 03.03.2022 17:31, Rahul Singh wrote:
> >>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct 
> >>>>>>>> vpci_msix *msix)
> >>>>>>>> 
> >>>>>>>>  return 0;
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>>>> +{
> >>>>>>>> +    struct domain *d = pdev->domain;
> >>>>>>>> +    unsigned int i;
> >>>>>>>> +
> >>>>>>>> +    if ( !pdev->vpci->msix )
> >>>>>>>> +        return 0;
> >>>>>>>> +
> >>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. 
> >>>>>>>> */
> >>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>>>> +    {
> >>>>>>>> +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, 
> >>>>>>>> i));
> >>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, 
> >>>>>>>> i) +
> >>>>>>>> +                                     vmsix_table_size(pdev->vpci, 
> >>>>>>>> i) - 1);
> >>>>>>>> +
> >>>>>>>> +        for ( ; start <= end; start++ )
> >>>>>>>> +        {
> >>>>>>>> +            p2m_type_t t;
> >>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>>>> +
> >>>>>>>> +            switch ( t )
> >>>>>>>> +            {
> >>>>>>>> +            case p2m_mmio_dm:
> >>>>>>>> +            case p2m_invalid:
> >>>>>>>> +                break;
> >>>>>>>> +            case p2m_mmio_direct:
> >>>>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>>>> +                {
> >>>>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>>>> +                    break;
> >>>>>>>> +                }
> >>>>>>>> +                /* fallthrough. */
> >>>>>>>> +            default:
> >>>>>>>> +                put_gfn(d, start);
> >>>>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO 
> >>>>>>>> area\n",
> >>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>>>> +                return -EEXIST;
> >>>>>>>> +            }
> >>>>>>>> +            put_gfn(d, start);
> >>>>>>>> +        }
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    return 0;
> >>>>>>>> +}
> >>>>>>> 
> >>>>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>>>> functions like clear_identity_p2m_entry() may not currently be 
> >>>>>>> available
> >>>>>>> on Arm. But this doesn't make the code x86-specific.
> >>>>>> 
> >>>>>> I will maybe be wrong but what I understand from the code is that for 
> >>>>>> x86 
> >>>>>> if there is no p2m entries setup for the region, accesses to them will 
> >>>>>> be trapped 
> >>>>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>>>> 
> >>>>>> But for ARM when we are registering the MMIO handler we have to 
> >>>>>> provide 
> >>>>>> the GPA also for the MMIO handler. 
> > 
> > Right, but you still need those regions to not be mapped on the second
> > stage translation, or else no trap will be triggered and thus the
> > handlers won't run?
> > 
> > Regardless of whether the way to register the handlers is different on
> > Arm and x86, you still need to assure that the MSI-X related tables
> > are not mapped on the guest second stage translation, or else you are
> > just allowing guest access to the native ones.
> 
> What I understand from the VPCI code we are not mapping the MSI-X related 
> tables/BAR
> to Stage-2 translation therefore no need to remove the mapping.
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/vpci/header.c;h=a1c928a0d26f5cefd98727ee37f5dc43ceba80a6;hb=HEAD#l248

Right, sorry, was slightly confused. So this is indeed only needed if
Arm does some kind of pre-mapping of non-RAM regions. For example an
x86 PVH dom0 will add the regions marked as 'reserved' to the second
stage translation, and we need vpci_make_msix_hole in order to punch
holes there if those pre-mapped regions happen to overlap with any
MSI-X table.

If there aren't any non-RAM regions mapped on Arm for it's hardware
domain by default then I guess it's safe to make this arch-specific.

Thanks, Roger.