Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000

["Bryan O'Donoghue" <pure.logic@nexus-software.ie>]

On 31/12/14 15:05, Andy Shevchenko wrote:

<snip>
>> +int imr_del(int reg, unsigned long base, unsigned long size);
>
> Same comments as for add_range.
> Could it be imr_remove_range() ?

Can be. You've actually caught me out there, function name is "imr_del"
function description says imr_del_range(). Amazing to think I proof read
this file a number of times for exactly thing type of thing

I'm happy with

imr_add_range();
imr_remove_range();

>> +
>> +#endif /* _IMR_H */
>> diff --git a/arch/x86/include/asm/intel-quark.h
b/arch/x86/include/asm/intel-quark.h
>> new file mode 100644
>> index 0000000..f51ac8c
>> --- /dev/null
>> +++ b/arch/x86/include/asm/intel-quark.h

<snip>

>> +#ifdef CONFIG_X86_32
>> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
>> +{
>> +       return (c->x86_vendor == X86_VENDOR_INTEL &&
>> +               c->x86 == 5 && c->x86_model == 9);
>> +}
>> +#else
>> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>> +#endif /* _ASM_X86_INTEL_QUARK_H */
>
> Could we use x86_match_cpu() instead?

I don't see why not. I'll kill this header and call out to
x86_match_cpu() in the Galileo and IMR code.

>> +       ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
>> +                               reg, &imr->addr_lo);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
>> +                               ++reg, &imr->addr_hi);
>
> Could you use reg++ in previous line and so on?

Yes I could. Originally wrote the code with a pre-increment and then
changed the flow later on. Post-increment will work just as good now and
is more readable.

> One more idea, if you make a union inside the structure you may do
> this in a loop.
> struct imr {
>  union {
>   struct imr {
>  ...
>   };
>   u32 regs[IMR_NUM_REGS];
>  };
> }
> Mostly same for _write().

Since reg is incrementing on each iosf_mbi_dothing() that can work.

>> +       u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
>> +       u32 reg_lock = reg;
>
> Do we need a separate variable? Would (reg - IMR_NUM_REGS) work for you?

Sure. A separate integer isn't required, I can make that change.

>> +done:
>> +       local_irq_restore(flags);
>
> Could you do like
>
>       local_irq_restore(flags);
>       return 0;
> done:
>       local_irq_restore(flags);
>       WARN(...)
>      return ret;
> ?

Doesn't change the logic so yeah, works for me.

>> +static int imr_dbgfs_state_show(struct seq_file *s, void *unused)
>> +{
>> +       int i, ret = -ENODEV;
>> +       struct imr imr;
>> +       u32 size;
>> +
>> +       mutex_lock(&imr_dev.lock);
>
> It seems you may get the imr_dev via parameters. I suggest to avoid
> using global variables as much as possible.

Appreciated, you're right globals are bad.

I think in this case the mutex at that scope is necessary though. Same
with the debugfs handle.

>> +
>> +       for (i = 0; i <= imr_dev.num; i++) {
>
> num is not num, but last one? Sounds confusing for me.

Will change.

>> +
>> +       f = debugfs_create_file("state", S_IFREG | S_IRUGO,
>> +                               dir, imr_dev, &imr_state_ops);
>> +       if (!f)
>> +               goto err;
>
> Are you planing to extend the debugfs contents? Would it be okay to
> use only one file for now?

No not planning to extend.
OK with the one file.

>> +}
>
> No need to put this function under ifdef  - debugfs has the stubs.

Ah - wasn't aware of that.
Great stuff - I'll kill the ifdefs - thanks !

> Can you define pr_fmt() ?
>
> 1 KiB.

Yep will do
:g/pr_info/s//pr_fmt/g

>
>> +                       base, size);
>> +               dump_stack();
>
> dump_stack is really needed here? In that case why not to use WARN()?

Hmm - I nope - dump_stack() isn't relevant since it's an in-kernel API.
Did an unthinking copy/past from the mtrr code

I'll zap that.

>> +       if (imr_check_range(base, size))
>> +               return -EINVAL;
>
> ret = imr_();
> if (ret)
>  return ret;

Good catch.

>> +       for (i = 0; i <= imr_dev.num; i++) {
>> +               ret = imr_read(i, &imr);
>> +               if (ret)
>> +                       goto done;
>> +
>> +               /* Find overlap */
>> +               if (imr_enabled(&imr)) {
>> +                       if (base >= imr_to_phys(imr.addr_lo) &&
>> +                               base <= imr_to_phys(imr.addr_hi)) {
>
>> +                               overlap = 1;
>> +                               break;
>
> Maybe
> ret = -EINVAL;
> goto done;

Agree - I like that change.
Waste of time to complete the loop if an overlap is detected.


>> +       if (reg >= 0) {
>> +               ret = imr_read(reg, &imr);
>> +               if (ret)
>> +                       goto done;
>> +
>> +               if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
>> +                       ret = -ENODEV;
>> +                       goto done;
>> +               }
>> +               found = 1;
>
> You may put a loop to the else branch instead of checking found at
> each iteration.

Sure.

> Happy New Year!

Thanks for the feedback Andy !
Best wishes in the New Year.

BOD