Re: [Xen-devel] [RFC PATCH] arm64: vgic-v3: Add ITS doorbell region in Dom0 stage-2

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 18/01/18 17:25, Manish Jaggi wrote:
> On 01/18/2018 10:22 PM, Julien Grall wrote:
> > Hi Manish,
> Hi Julien,
> > On 18/01/18 06:15, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> > > From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>
> > >
> > > This patch introduces a function vgic_map_translation_space for mapping
> > > ITS 64K translater space for doorbells in dom0 stage-2.
> > > It is required as dom0 PCI devices behined SMMU  wont be able to 
> > > write MSIs.
> > s/behined/behind/
> >
> > also there a two spaces after SMMU. One should be enough.
> ok
> > > This function is called from vgic_v3_its_init_domain only for 
> > > hardware domain.
> > This is really confusing. You mix dom0 and hardware domain in the same 
> > commit. Please use only "hardware domain".
> I stressed this here as this function is only called for hardware 
> domain, if you see line 1550
So why are you using the word Dom0 everywhere else? I keep asking you to 
use the term "Hardware Domain".
> |if ( is_hardware_domain(d) ) is checked.|
>
> > > This patch is also required for testing
> > > [RFC 00/11] acpi: arm: IORT Support for Xen [1]
> > > + [2]  [RFC v4 0/8] SMMUv3 driver
> > >   [1] 
> > > https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00007.html
> > > [2] https://lists.xen.org/archives/html/xen-devel/2017-12/msg01294.html
> > Anything after "This patch..." does not seem to be relevant in the 
> > commit. You might want to add that after --- so it get stripped after 
> > committing.
> ok
> > > Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> > > ---
> > >   xen/arch/arm/vgic-v3-its.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> > > index 72a5c70656..a8f8079149 100644
> > > --- a/xen/arch/arm/vgic-v3-its.c
> > > +++ b/xen/arch/arm/vgic-v3-its.c
> > > @@ -1511,6 +1511,30 @@ unsigned int vgic_v3_its_count(const struct 
> > > domain *d)
> > >       return ret;
> > >   }
> > >   +static int vgic_map_translation_space(struct domain *d, paddr_t 
> > > guest_addr)
> > This function is quite confusing. What is guest_addr? It looks like it 
> > is the base address of the ITS but it is not very clear here.
> ok I will change the name.
> > > +{
> > > +    u64 addr, size;
> > This should be paddr_t.
> >
> > > +    int ret;
> > Newline here please.
> >
> > > +    addr = guest_addr + SZ_64K;
> > > +    size = SZ_64K;
> > > +
> > > +    ret = map_mmio_regions(d,
> > > +                           paddr_to_pfn(addr & PAGE_MASK),
> > > +                           DIV_ROUND_UP(size, PAGE_SIZE),
> > > +                           paddr_to_pfn(addr & PAGE_MASK));
> > You are assuming a 1:1 mapping in the domain. Nowhere you explain that 
> > assumption nor that it will not work for guest. At least you need an 
> > ASSERT(is_domain_direct_mapped(d)).
> > Furthermore, as we discussed yesterday I would expect a big fat 
> > comment that this need to be revisited when DomU support will be added 
> > as it may not be safe to map the Doorbell in page-tables used by the 
> > processor.
> I think you didnt read the commit log properly, this function is *only* 
> called for hardware domain. So there is no question of DomU.
> Request you to please see the code again.
I read properly the commit log... and my point still stands. This 
function is name vgic_map_translation_space. How a developer would guess 
that it can't be used for other domain than the hardware domain and 
direct mapped? More that you pass a domain pointer...
> > >   /*
> > >    * For a hardware domain, this will iterate over the host ITSes
> > >    * and map one virtual ITS per host ITS at the same address.
> > > @@ -1541,6 +1565,8 @@ int vgic_v3_its_init_domain(struct domain *d)
> > >                   return ret;
> > >               else
> > >                   d->arch.vgic.has_its = true;
> > > +
> > > +            vgic_map_translation_space(d, hw_its->addr);
> > Your new function may return an error. Therefore you should test it.
> yes, I thought we will focus more is this is the right place to add this 
> test code.
This is not difficult to check error return in a first draft. Please 
stop sending patch that are full of coding style error or half done 
(without even mentioning it).
This is rather annoying and a way to miss that afterwards.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel