Hi Roger,
> On 9 Mar 2022, at 10:16 am, Roger Pau Monné <roger....@citrix.com> 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 <jbeul...@suse.com> wrote:
>>>
>>> On 03.03.2022 17:31, Rahul Singh wrote:
>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeul...@suse.com> wrote:
>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeul...@suse.com> 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.
