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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Rahul Singh <Rahul.Singh@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Paul Durrant <paul@xen.org>
Subject: Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file
Date: Wed, 9 Mar 2022 11:16:06 +0100	[thread overview]
Message-ID: <Yih+ZjxKz0SMOXef@Air-de-Roger> (raw)
In-Reply-To: <1205992D-ACE4-4E94-B554-3B14F6D9697E@arm.com>

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@suse.com> wrote:
> > 
> > On 03.03.2022 17:31, Rahul Singh wrote:
> >>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> >>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@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.

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?

Thanks, Roger.