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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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

> 
> So you do need this function on Arm in order to prevent hardware MSI-X
> tables being accessed by the guest. Or are you suggesting it's
> intended for Arm guest to access the native MSI-X tables?

On ARM also access to the MSI-X tables will be trapped and physical MSI-X table
will be updated accordingly.

Regards,
Rahul
> 
> Thanks, Roger.