Re: [kvm-unit-tests PATCH v7 06/13] arm/arm64: ITS: Introspection tests

[Andrew Jones]

On Mon, Mar 30, 2020 at 11:56:00AM +0200, Auger Eric wrote:
> Hi,
> 
> On 3/30/20 11:11 AM, Andrew Jones wrote:
> > On Mon, Mar 30, 2020 at 10:46:57AM +0200, Auger Eric wrote:
> >> Hi Zenghui,
> >>
> >> On 3/30/20 10:30 AM, Zenghui Yu wrote:
> >>> Hi Eric,
> >>>
> >>> On 2020/3/20 17:24, Eric Auger wrote:
> >>>> +static void its_cmd_queue_init(void)
> >>>> +{
> >>>> +    unsigned long order = get_order(SZ_64K >> PAGE_SHIFT);
> >>>> +    u64 cbaser;
> >>>> +
> >>>> +    its_data.cmd_base = (void *)virt_to_phys(alloc_pages(order));
> >>>
> >>> Shouldn't the cmd_base (and the cmd_write) be set as a GVA?
> >> yes it should
> > 
> > If it's supposed to be a virtual address, when why do the virt_to_phys?
> What is programmed in CBASER register is a physical address. So the
> virt_to_phys() is relevant. The inconsistency is in its_allocate_entry()
> introduced later on where I return the physical address instead of the
> virtual address. I will fix that.
> 
> 
> > 
> >>>
> >>> Otherwise I think we will end-up with memory corruption when writing
> >>> the command queue.  But it seems that everything just works fine ...
> >>> So I'm really confused here :-/
> >> I was told by Paolo that the VA/PA memory map is flat in kvmunit test.
> > 
> > What does flat mean?
> 
> Yes I meant an identity map.
> 
>  kvm-unit-tests, at least arm/arm64, does prepare
> > an identity map of all physical memory, which explains why the above
> > is working.
> 
> should be the same on x86

Maybe, but I didn't write or review how x86 does their default map, so I
don't know.

> 
>  It's doing virt_to_phys(some-virt-addr), which gets a
> > phys addr, but when the ITS uses it as a virt addr it works because
> > we *also* have a virt addr == phys addr mapping in the default page
> > table, which is named "idmap" for good reason.
> > 
> > I think it would be better to test with the non-identity mapped addresses
> > though.
> 
> is there any way to exercise a non idmap?

You could create your own map and then switch to it. See lib/arm/asm/mmu-api.h

But, you don't have to switch the whole map to use non-identity mapped
addresses. Just create new virt mappings to the phys addresses you're
interested in, and then use those new virt addresses instead. If you're
worried that somewhere an identity mapped virt address is getting used
because of a phys/virt address mix up, then you could probably sprinkle
some assert(virt_to_phys(addr) != addr)'s around to ensure it.

Thanks,
drew

> 
> Thanks
> 
> Eric
> > 
> > Thanks,
> > drew
> > 
> >>
> >>>
> >>>> +
> >>>> +    cbaser = ((u64)its_data.cmd_base | (SZ_64K / SZ_4K - 1)    |
> >>>> GITS_CBASER_VALID);
> >>>> +
> >>>> +    writeq(cbaser, its_data.base + GITS_CBASER);
> >>>> +
> >>>> +    its_data.cmd_write = its_data.cmd_base;
> >>>> +    writeq(0, its_data.base + GITS_CWRITER);
> >>>> +}
> >>>
> >>> Otherwise this looks good,
> >>> Reviewed-by: Zenghui Yu <address@hidden>
> >> Thanks!
> >>
> >> Eric
> >>>
> >>>
> >>> Thanks
> >>>
> >>
> >>
> 
>