Re: [Xen-devel] [PATCH V3 26/29] x86/vvtd: Handle interrupt translation faults

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 20, 2017 at 01:54:15PM +0800, Chao Gao wrote:
> On Thu, Oct 19, 2017 at 05:31:37PM +0100, Roger Pau Monné wrote:
> >On Thu, Sep 21, 2017 at 11:02:07PM -0400, Lan Tianyu wrote:
> >> +static int vvtd_alloc_frcd(struct vvtd *vvtd)
> >> +{
> >> +    int prev;
> >> +    uint64_t cap = vvtd_get_reg(vvtd, DMAR_CAP_REG);
> >> +    unsigned int base = cap_fault_reg_offset(cap);
> >> +
> >> +    /* Set the F bit to indicate the FRCD is in use. */
> >> +    if ( !vvtd_test_and_set_bit(vvtd,
> >> +                                base + vvtd->status.fault_index * 
> >> DMA_FRCD_LEN +
> >> +                                DMA_FRCD3_OFFSET, DMA_FRCD_F_SHIFT) )
> >> +    {
> >> +        prev = vvtd->status.fault_index;
> >> +        vvtd->status.fault_index = (prev + 1) % cap_num_fault_regs(cap);
> >> +        return vvtd->status.fault_index;
> >
> >I would prefer that you return the index as an unsigned int parameter
> >passed by reference rather than as the return value of the function,
> >but that might not be the preference of others.
> 
> What are the pros and cons?

I personally don't like return values that have different meanings
depending on it's sign. Here < 0 means error, while >= 0 is used to
deliver some information.

What I didn't like here specifically (apart from the rant above) is
that I would prefer index to be unsigned int, but I'm not sure that's
enough to ask you to change the function prototype. Just leave it
as-is unless someone else complains.

Thanks, Roger.

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