Re: [Xen-devel] [PATCH v4 11/15] x86/altp2m: define and implement alternate p2m HVMOP types.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.07.15 at 00:03, <ravi.sahita@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>Sent: Friday, July 10, 2015 3:01 AM
>>>>> On 10.07.15 at 02:52, <edmund.h.white@xxxxxxxxx> wrote:
>>> +        default:
>>> +            return -ENOSYS;
>>> +
>>> +            break;
>>
>>Bogus (unreachable) break.
> 
> Wanted to keep this so that if someone removes the error code then they 
> don't cause an invalid fall through.
> But ok with removing it if you think so.

We don't (intentionally) do this anywhere else, so it should be
removed.

>>> +                if ( !(d ? d : current->domain)->arch.altp2m_active )
>>
>>This is bogus: d is NULL if and only if altp2m_vcpu_enable_notify, i.e. I 
> don't
>>see why you can't just use current->domain inside that case (and you really
>>do). That would then also eliminate the need for this redundant and
>>obfuscating switch() nesting you use.
>>
> 
> We need to check if the target domain is in altp2m mode for all the 
> following sub-ops.
> If we removed this check, we would need to check for target domain being in 
> altp2m for all the following cases.
> Andrew wanted to refactor and pull common code up, and that's what this is 
> one case of for hvm_op.

I'd be fine with such refactoring if it didn't result in nested switch()-es
using the same control expression.

>>> +
>>> +struct xen_hvm_altp2m_set_mem_access {
>>> +    /* view */
>>> +    uint16_t view;
>>> +    /* Memory type */
>>> +    uint16_t hvmmem_access; /* xenmem_access_t */
>>> +    uint8_t pad[4];
>>> +    /* gfn */
>>> +    uint64_t gfn;
>>> +};
>>> +typedef struct xen_hvm_altp2m_set_mem_access
>>> xen_hvm_altp2m_set_mem_access_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>> +
>>> +struct xen_hvm_altp2m_change_gfn {
>>> +    /* view */
>>> +    uint16_t view;
>>> +    uint8_t pad[6];
>>> +    /* old gfn */
>>> +    uint64_t old_gfn;
>>> +    /* new gfn, INVALID_GFN (~0UL) means revert */
>>> +    uint64_t new_gfn;
>>> +};
>>> +typedef struct xen_hvm_altp2m_change_gfn
>>xen_hvm_altp2m_change_gfn_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t);
>>> +
>>> +struct xen_hvm_altp2m_op {
>>> +    uint32_t cmd;
>>> +/* Get/set the altp2m state for a domain */
>>> +#define HVMOP_altp2m_get_domain_state     1
>>> +#define HVMOP_altp2m_set_domain_state     2
>>> +/* Set the current VCPU to receive altp2m event notifications */
>>> +#define HVMOP_altp2m_vcpu_enable_notify   3
>>> +/* Create a new view */
>>> +#define HVMOP_altp2m_create_p2m           4
>>> +/* Destroy a view */
>>> +#define HVMOP_altp2m_destroy_p2m          5
>>> +/* Switch view for an entire domain */
>>> +#define HVMOP_altp2m_switch_p2m           6
>>> +/* Notify that a page of memory is to have specific access types */
>>> +#define HVMOP_altp2m_set_mem_access       7
>>> +/* Change a p2m entry to have a different gfn->mfn mapping */
>>> +#define HVMOP_altp2m_change_gfn           8
>>> +    domid_t domain;
>>> +    uint8_t pad[2];
>>
>>While you added padding fields as asked for, you still don't verify them to 
> be
>>zero on input.
> 
> Specifically what were you thinking we need to do here - also would be good 
> if you can explain what was the underlying concern? (thanks)

I'm pretty sure I said so before - future extensibility. I.e. a means to
make use of the now unused (padding) fields, which can only be done
if the fields are being checked to be zero while unused.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel