Re: [PATCH v2] [Qemu-devel] target/i386: HAX: Enable ROM/ROM device memory region support

[Philippe Mathieu-Daudé]

On 4/28/20 4:45 AM, Colin Xu wrote:
> Hi Paolo,
>
> Would you please queue this one?
> --
> Best Regards,
> Colin Xu
>
> On Mon, 30 Mar 2020, Colin Xu wrote:
>
> > Looks good to me.
> >
> > Reviewed-by: Colin Xu <address@hidden>
> >
> > On 2020-03-30 11:25, address@hidden wrote:
> > > From: Hang Yuan <address@hidden>
> > >
> > > Add ROM and ROM device memory region support in HAX. Their memory 
> > > region is
> > > read only and write access will generate EPT violation. The violation 
> > > will be
> > > handled in the HAX kernel with the following patch.

"will be"? This patch is 10 months old.

> > > https://github.com/intel/haxm/commit/33ceea09a1655fca12c47f1e112b1d269357ff28 
> > >
> > >
> > > v2: fix coding style problems

^ This line goes ...

> > > Signed-off-by: Hang Yuan <address@hidden>
> > > ---

... here after the '---'.

> > >   target/i386/hax-mem.c | 11 ++++-------
> > >   1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/target/i386/hax-mem.c b/target/i386/hax-mem.c
> > > index 6bb5a24917..a8bfd37977 100644
> > > --- a/target/i386/hax-mem.c
> > > +++ b/target/i386/hax-mem.c
> > > @@ -175,13 +175,10 @@ static void 
> > > hax_process_section(MemoryRegionSection *section, uint8_t flags)
> > >       uint64_t host_va;
> > >       uint32_t max_mapping_size;
> > >
> > > -    /* We only care about RAM and ROM regions */
> > > -    if (!memory_region_is_ram(mr)) {
> > > -        if (memory_region_is_romd(mr)) {
> > > -            /* HAXM kernel module does not support ROMD yet  */
> > > -            warn_report("Ignoring ROMD region 0x%016" PRIx64 
> > > "->0x%016" PRIx64,
> > > -                        start_pa, start_pa + size);

Don't you need to check for some kmod version before removing this check?

> > > -        }
> > > +    /* We only care about RAM/ROM regions and ROM device */
> > > +    if (memory_region_is_rom(mr) || (memory_region_is_romd(mr))) {

Redundant parenthesis.

> > > +        flags |= HAX_RAM_INFO_ROM;
> > > +    } else if (!memory_region_is_ram(mr)) {
> > >           return;
> > >       }

If you move the 'if (RAM) return' first, the code becomes easier to review.

> > >
> >
> > --
> > Best Regards,
> > Colin Xu