Re: [PATCH] add HVMOP_get_mem_type hvmop

[Olaf Hering <olaf@aepfle.de>]

On Tue, May 03, Tim Deegan wrote:

> > +        rc = rcu_lock_remote_target_domain_by_id(a.domid, &d);
> 
> I thought this call was intended to be used from inside the guest in
> question.  rcu_lock_remote_target_domain_by_id() explicitly refuses to
> let a domain operate on itself. 

Hmm, I have to test it with xen-unstable too. I did copy&paste from
other parts of the file. 4.0 used rcu_lock_target_domain_by_id, so can I
use that function?



> > +typedef enum {
> > +    HVMMEM_ram_rw,             /* Normal read/write guest RAM */
> > +    HVMMEM_ram_ro,             /* Read-only; writes are discarded */
> > +    HVMMEM_mmio_dm,            /* Reads and write go to the device model */
> > +} hvmmem_type_t;
> > +
> 
> This is now outside the #ifdef, when both of its users are inside it.
> If that wasn't deliberate, please put it back. 

Should HVMOP_get_mem_type use hvmmem_type_t? Its outside the ifdef,
otherwise unmodified_drivers/linux-2.6/platform-pci/platform-pci.c does
not get the define.

> >  /* Following tools-only interfaces may change in future. */
> >  #if defined(__XEN__) || defined(__XEN_TOOLS__)
> >  
> > @@ -109,11 +115,6 @@ typedef struct xen_hvm_modified_memory x
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_modified_memory_t);
> >  
> >  #define HVMOP_set_mem_type    8
> > -typedef enum {
> > -    HVMMEM_ram_rw,             /* Normal read/write guest RAM */
> > -    HVMMEM_ram_ro,             /* Read-only; writes are discarded */
> > -    HVMMEM_mmio_dm,            /* Reads and write go to the device model */
> > -} hvmmem_type_t;
> >  /* Notify that a region of memory is to be treated in a specific way. */
> >  struct xen_hvm_set_mem_type {
> >      /* Domain to be updated. */
> > @@ -223,6 +224,20 @@ struct xen_hvm_inject_trap {
> >  typedef struct xen_hvm_inject_trap xen_hvm_inject_trap_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_trap_t);
> >  
> > +#define HVMOP_get_mem_type    15
> > +/* Return hvmmem_type_t for the specified pfn. */
> > +struct xen_hvm_get_mem_type {
> > +    /* Domain to be queried. */
> > +    domid_t domid;
> > +    /* OUT variable. */
> > +    uint8_t mem_type;
> > +    /* IN variable. */
> > +    uint64_t pfn;
> 
> This structure will be laid out differently on 32-bit and 64-bit
> builds. :(  Also, since the _set operation uses a 16-bit variable for
> the type, you might as well do the same here. 

Good point. Should I use the same padding as in xen_hvm_pagetable_dying?

struct xen_hvm_get_mem_type {
    /* Domain to be queried. */
    domid_t domid;
    /* OUT variable. */
    uint16_t mem_type;
    uint16_t pad[2];
    /* IN variable. */
    uint64_t pfn;
};




Olaf