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....@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
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.
